Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding a test with cppcheck for Travis CI #56

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Adding a test with cppcheck for Travis CI #56

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 1, 2016

No description provided.

brarcher
brarcher reviewed Jul 2, 2016
View reviewed changes
src/check_str.c
@@ -127,9 +127,8 @@ static int percent_passed(TestStats * t)
{
if(t->n_failed == 0 && t->n_errors == 0)
return 100;
else if(t->n_checked == 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these style changes or logic changes? Are these because cppcheck indicated an issue?

As least for cppcheck 1.74 no issues were detected in the src dir. The test dir did mention the following:

[tests/check_check_pack.c:323] -> [tests/check_check_pack.c:328]: (warning) Either the condition 'rmsg!=0' is redundant or there is possible null pointer dereference: rmsg.

Which seems to indicate that there is a redundant null check. The following resolves this:

diff --git a/tests/check_check_pack.c b/tests/check_check_pack.c
index 7da7e57..dd5d557 100644
--- a/tests/check_check_pack.c
+++ b/tests/check_check_pack.c
@@ -325,7 +325,6 @@ START_TEST(test_ppack_onlyctx)
   ck_assert_msg (rmsg->test_line == -1,
               "Result loc line should be -1 with only CTX");

-  if (rmsg != NULL)
     free (rmsg);
   fclose(result_file);
 }

Would you mind including this into your pull request, to remove all errors/warnings before enabling the check?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it is very small modification, so I putted it with cppcheck. else is useless as return in the previous exit the function if the test is true.

@brarcher
Copy link
Contributor

brarcher commented Jul 2, 2016

Interesting, I've not heard of cppcheck. It sounds good to give it a shot and see how it goes.

This project currently uses Clang's static code analysis tool in a Jenkins job here. I wonder if cppcheck will be able to catch other issues which Clang's tool does not.

@brarcher
Copy link
Contributor

brarcher commented Jul 2, 2016

This part is a formality, so that the Jenkins-hosted automated tests will run:

Jenkins: ok to test

@ghost
Copy link
Author

ghost commented Jul 4, 2016

You may also be interested by vera++: https://bitbucket.org/verateam/vera/overview
An example: https://gitlab.com/RyDroid/PlanetWars2dRT-core/blob/master/tools/vera++-tests.sh

@brarcher
Copy link
Contributor

Are you still interested in getting this change in? If so, could you update the branch so that it can be evaluated against master?

@ghost
Copy link
Author

ghost commented Dec 14, 2016

I am still interested. I put the commit on top.

@brarcher
Copy link
Contributor

I do not think that the change was successful on Linux. The following was reported on Travis-CI:

$ if [ ${TRAVIS_OS_NAME} = 'linux' ]; then cppcheck --verbose --quiet --error-exitcode=1 --force --enable=performance,portability src/ tests/; fi
[tests/check_check_sub.c:846]: (error) Division by zero
[tests/check_check_sub.c:858]: (error) Division by zero
[tests/check_check_sub.c:869]: (error) Division by zero
[tests/check_check_sub.c:894]: (error) Division by zero
[tests/check_check_sub.c:1316]: (error) Division by zero
[tests/check_check_sub.c:1328]: (error) Division by zero
[tests/check_check_sub.c:1339]: (error) Division by zero
[tests/check_check_sub.c:1364]: (error) Division by zero
[tests/check_check_sub.c:1788]: (error) Division by zero
[tests/check_check_sub.c:1800]: (error) Division by zero
[tests/check_check_sub.c:1811]: (error) Division by zero
[tests/check_check_sub.c:1836]: (error) Division by zero
The command "if [ ${TRAVIS_OS_NAME} = 'linux' ]; then cppcheck --verbose --quiet --error-exitcode=1 --force --enable=performance,portability src/ tests/; fi" exited with 1.

cppcheck was smart enough to determine that there were some division by zeros, for example:

  float x = 0.0001f;
  ck_assert_float_finite(x);
  // MS VS doesn't allow explicit division by zero
  float t = 1.0f;
  x = 1.0f / (1.0f - t);
  record_failure_line_num(__LINE__);
  ck_assert_float_finite(x);

However, in this case it is intentional, as the test checks that the result is INF.

Is there a way to disable the divide by zero check?

@ghost
Copy link
Author

ghost commented Dec 22, 2016

The version of cppcheck of Debian Jessie returns a different output:

$ cppcheck --verbose --quiet --error-exitcode=1 --force --enable=performance,portability src/ tests/
[tests/check_check_sub.c:656] -> [tests/check_check_sub.c:658]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:660] -> [tests/check_check_sub.c:664]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:666] -> [tests/check_check_sub.c:668]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:670] -> [tests/check_check_sub.c:672]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:674] -> [tests/check_check_sub.c:676]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:820] -> [tests/check_check_sub.c:822]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:824] -> [tests/check_check_sub.c:828]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:830] -> [tests/check_check_sub.c:832]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1126] -> [tests/check_check_sub.c:1128]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1130] -> [tests/check_check_sub.c:1134]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1136] -> [tests/check_check_sub.c:1138]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1140] -> [tests/check_check_sub.c:1142]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1144] -> [tests/check_check_sub.c:1146]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1290] -> [tests/check_check_sub.c:1292]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1294] -> [tests/check_check_sub.c:1298]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1300] -> [tests/check_check_sub.c:1302]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1598] -> [tests/check_check_sub.c:1600]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1602] -> [tests/check_check_sub.c:1606]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1608] -> [tests/check_check_sub.c:1610]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1612] -> [tests/check_check_sub.c:1614]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1616] -> [tests/check_check_sub.c:1618]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1762] -> [tests/check_check_sub.c:1764]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1766] -> [tests/check_check_sub.c:1770]: (performance) Variable 'i' is reassigned a value before the old one has been used.
[tests/check_check_sub.c:1772] -> [tests/check_check_sub.c:1774]: (performance) Variable 'i' is reassigned a value before the old one has been used.

I can not check currently on an other OS (but I will be able on Trisquel, a fork of Ubuntu, in January). But it seems to be possible: https://stackoverflow.com/questions/2956331/how-to-use-cppchecks-inline-suppression-filter-option-for-c-code

@brarcher
Copy link
Contributor

Interesting. Perhaps the version of cppcheck on Travis-CI is a more recent version, or at least a different version?

If we were able to filter some of the items at will, then cppcheck may make a good addition. Let me know if you find time to check that out further. Further, feel free to push changes to this branch and see how Travis-CI runs with them, if it is helpful.

@mikkoi
Copy link
Contributor

mikkoi commented Sep 19, 2019

I added a Cppcheck run into the CMake build. #222

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@brarcher @mikkoi